Adding simple autorecover - hopefully all of my build errors are fixed #249

watters-torc · 2017-09-12T17:56:44Z

Maintainers: This is a feature we needed for a project, originally we were under the impression the socketcan_bridge and socketcan_interface were under BSD, it seems like this project (ros_canopen) has a bunch of mixed licenses. I don't necessarily see this as a production ready fix but it works for what we need. I'm able to make changes or take direction from the maintainers if you'd like work to further be done here.

Proposed fix / gap stop for #244

…tead

mathias-luedtke

First all: Thanks for your contribution!
Please find my immediate change requests below.

In ros_canopen auto-recovering should not be done by default.
So please add a parameter to enable this feature explicitly.
It would be great to have a Trigger service that could be called e.g. in non-auto mode.

PS: You can add changes in this PR, it will update automatically. There is no need to open new PRs.

mathias-luedtke · 2017-09-22T18:41:13Z

socketcan_bridge/CMakeLists.txt

@@ -1,4 +1,5 @@
 cmake_minimum_required(VERSION 2.8.3)
+add_compile_options(-std=c++11)


C++11 shall not be used!

Sigh. It's 2018 i.e. 2 more standards later.

c++14 is now the default for melodc 🎉

mathias-luedtke · 2017-09-22T18:41:29Z

socketcan_bridge/CMakeLists.txt

  ${catkin_LIBRARIES}
 )

+


Is there a .clang-format file which can be used to avoid this kind of error?

mathias-luedtke · 2017-09-22T18:42:00Z

socketcan_bridge/CMakeLists.txt

    socketcan_to_topic
    socketcan_to_topic_node
    topic_to_socketcan
    topic_to_socketcan_node
+


mathias-luedtke · 2017-09-22T18:42:17Z

socketcan_bridge/CMakeLists.txt

@@ -131,4 +151,6 @@ if(CATKIN_ENABLE_TESTING)
    ${catkin_LIBRARIES}
  )

+


whitespace, please keep the new line ending

mathias-luedtke · 2017-09-22T18:43:48Z

socketcan_bridge/src/socketcan_recover_ctrl.cpp

+    {
+        state_pub_ = nh->advertise<can_msgs::CanState>("can_state", 1, true);
+
+        timer_ = nh->createWallTimer(ros::WallDuration(5), [this](const ros::WallTimerEvent& event) {recover();}, true, false);


please migrate the lambda into a dedicated function

It might be good to make the timeout configurable.

mathias-luedtke · 2017-09-22T18:45:41Z

socketcan_bridge/src/socketcan_recover_ctrl.cpp

+        }
+        else
+        {
+            timer_.stop();


Why do you need to stop a one-shot timer?

If status has been updated before the callback fires, we we're giving a little time for this system to "maybe" fix itself

void ros::WallTimer::stop ()
| Stop the timer. Once this call returns, no more callbacks will be called. Does nothing if the timer is already stopped.

ie, if status became ready we don't want the callback to fire

There is no way that the state can change from bad to good without intervention (?).

I wasn't sure if that statement would hold true in future versions of socketcan_interface

mathias-luedtke · 2017-09-22T18:46:05Z

socketcan_bridge/src/socketcan_recover_ctrl.cpp

+     */
+    void SocketCANRecoverCtrl::recover() 
+    {
+        timer_.stop();


no need to stop here

This stop is explicitly required, you will have to forgive me I don't remember why, either it made the timer hang holding a reference preventing from being able to simple Ctrl-C the app, or it was needed to call start below, ill look into my reasoning here

Ah yes, it might block at ctrl+c.
But fixing it here is just handling the symptom.

mathias-luedtke · 2017-09-22T18:47:28Z

socketcan_bridge/src/socketcan_recover_ctrl.cpp

+            default:
+                state_msg.driver_state = can_msgs::CanState::CLOSED;
+                break;
+        }


I would prefer to just copy it: state_msg.driver_state = state.driver_state;

can::State and can_msgs::CanState are incompatible, i think

mathias-luedtke · 2017-09-22T18:47:52Z

socketcan_interface/include/socketcan_interface/socketcan.h

@@ -149,7 +149,7 @@ class SocketCANInterface : public AsioDriver<boost::asio::posix::stream_descript
            ret = true;
        }
        if( internal_error & CAN_ERR_RESTARTED){
-            str += "ontroller restarted;";
+            str += "controller restarted;";


mathias-luedtke · 2017-09-22T19:19:21Z

socketcan_bridge/src/socketcan_recover_ctrl.cpp

+     */
+    void SocketCANRecoverCtrl::stateCallback(const can::State & state) {
+        publishStatus(state);
+        if(!state.isReady())


I guess the driver should get recovered only if it is not closed.

mathias-luedtke · 2018-03-20T14:38:55Z

@watters-torc: Do you have time for working on it?

If not, I will see if I can find some time to implement the follwing base on part of your code:

diagnostics output for all bridge nodes (instead of the custom message)
recover service
opt-in auto-recover

redkite · 2018-05-24T11:51:10Z

Is this still work in progress? We also receive error frames which kill the bridge all the time.

koenlek · 2018-06-04T20:18:49Z

Works like a charm for us. Would be nice if this could make it into an official release

Erumeldor · 2019-01-14T10:18:43Z

Would be still a good thing to have a auto recovery when having errors. For my case, when disconnecting the can network from our hardware and reconnect it again, there is no time the can messages will be send again. It always says: "[ERROR] [...]: Failed to send message: 8...#00....."

tonybaltovski · 2020-10-26T17:43:45Z

Anything more needed to get this merged?

Joe Adkisson and others added 9 commits August 29, 2017 09:55

added a class to automatically recover from errors

a8badf8

Modifying to use status changed delegate to check for autorecover ins…

4a3955b

…tead

fixing spelling

abf64df

Removing non existent target can_srvs

7829852

Removing lingering dynamic reconfig that we don't really need

b0320b3

Addign timer_.stop() to prevent from hangning

add37de

Fixing build errors

2cc23f1

Fixing syntax build errors

43fde6d

removing license at top of files

3302da0

mathias-luedtke requested changes Sep 22, 2017

View reviewed changes

mathias-luedtke reviewed Sep 22, 2017

View reviewed changes

mathias-luedtke changed the base branch from jade-devel to kinetic-devel October 6, 2017 09:47

WasabiFan mentioned this pull request Feb 7, 2018

socketcan_bridge: unrecoverable error "controller problems;, asio: Success" #267

Open

mathias-luedtke mentioned this pull request Jun 1, 2018

topic_to_socketcan communication breaks under high load #285

Closed

mathias-luedtke added the wrid18 label Jul 11, 2018

matt-attack mentioned this pull request Nov 5, 2018

If an error occurs try and recover from it if necessary #310

Open

gavanderhoorn removed the wrid18 label Jun 28, 2019

BenArtes mentioned this pull request Sep 14, 2022

Added can device recovery code #469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding simple autorecover - hopefully all of my build errors are fixed #249

Adding simple autorecover - hopefully all of my build errors are fixed #249

watters-torc commented Sep 12, 2017 •

edited

Loading

mathias-luedtke left a comment

mathias-luedtke Sep 22, 2017

daixtrose Mar 1, 2018

mathias-luedtke May 24, 2018

mathias-luedtke Sep 22, 2017

daixtrose Mar 1, 2018

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

watters-torc Sep 22, 2017

watters-torc Sep 22, 2017

mathias-luedtke Sep 22, 2017

watters-torc Sep 22, 2017

mathias-luedtke Sep 22, 2017

watters-torc Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

watters-torc Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke Sep 22, 2017

mathias-luedtke commented Mar 20, 2018

redkite commented May 24, 2018

koenlek commented Jun 4, 2018

Erumeldor commented Jan 14, 2019

tonybaltovski commented Oct 26, 2020

		@@ -1,4 +1,5 @@
		cmake_minimum_required(VERSION 2.8.3)
		add_compile_options(-std=c++11)

		@@ -131,4 +151,6 @@ if(CATKIN_ENABLE_TESTING)
		${catkin_LIBRARIES}
		)

Adding simple autorecover - hopefully all of my build errors are fixed #249

Are you sure you want to change the base?

Adding simple autorecover - hopefully all of my build errors are fixed #249

Conversation

watters-torc commented Sep 12, 2017 • edited Loading

mathias-luedtke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathias-luedtke commented Mar 20, 2018

redkite commented May 24, 2018

koenlek commented Jun 4, 2018

Erumeldor commented Jan 14, 2019

tonybaltovski commented Oct 26, 2020

watters-torc commented Sep 12, 2017 •

edited

Loading